-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Disallow enums in ArrayObject #15775
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Isn't the whole point of |
I have no clue what the point is, tbh. I really don't understand when that would be useful. In any case, this allows to circumvent both type and readonly checks (https://3v4l.org/KOWT9), which can both violate assumptions. Types are obvious. Readonly can break internal enum name checks when the name of internal enums is changed. |
My point was more that instead of disallowing objects in And frankly, not only |
Indeed. Unfortunately, there's quite a bit of use of ArrayObject in the wild. For now, I'll check if it's possible to uphold property types and readonly for internal classes, where the actual assertions can occur. I don't object to deprecating ArrayObject in the future, but I also don't care if it no longer causes unsoundness. |
@cmb69 Having looked at this more, you were right. This class is pure madness. Basically none of the operations behave like they should. Some of the issues:
IMO, we should try to understand the use-cases online, see whether they have good alternatives, and then deprecate this class. |
Thanks for having a closer look at the implementation! I would presume that |
Agreed with @cmb69 that there should be more warnings and documentation of the limitations of |
Note that |
@cmb69 @nielsdos @bwoebi Given recent discussions on php-src/ext/random/randomizer.c Lines 161 to 176 in 1b9568d
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes imo.
Not sure about the wording as it seems to imply that only some particular enums are incompatible. I would change it to "Enums are not compatible (...)" rather than "Enum %s is not compatible".
I am fine with disallowing enums, as those are rather a recent addition and unlikely to have been used in conjunction with ArrayObject. |
I decided to only merge this for master. Woe is you if you're relying on this behavior. |
See <php/php-src#15775 (comment)>. While a formal deprecation process is pending, it seems prudent to not advertise this class any longer.
See <php/php-src#15775 (comment)>. While a formal deprecation process is pending, it seems prudent to not advertise this class any longer.
See <php/php-src#15775 (comment)>. While a formal deprecation process is pending, it seems prudent to not advertise this class any longer. php/doc-en@3994d01
We should probably disallow objects in
ArrayObject
altogether. It allows overridingreadonly
properties which can break engine assumptions.